Initial Numba support#626
Merged
Merged
Conversation
Contributor
|
21 fixed, 0 new since branch point (7409297) ✅ 21 CodeQL alerts resolved since the previous PR commit
✅ 21 CodeQL alerts resolved since the branch point
Review the full CodeQL report for details. |
Contributor
Clang-Tidy Check Results55 new issue(s) introduced by this patch (5842 total). New issues on modified lines:
All issues by check:
See inline comments for details. Comment |
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #626 +/- ##
==========================================
- Coverage 83.55% 82.95% -0.61%
==========================================
Files 170 172 +2
Lines 6283 6436 +153
Branches 706 742 +36
==========================================
+ Hits 5250 5339 +89
- Misses 810 855 +45
- Partials 223 242 +19
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Contributor
Clang-Tidy Check Results3 new issue(s) introduced by this patch (5777 total). New issues on modified lines:
All issues by check:
See inline comments for details. Comment |
Contributor
Clang-Tidy Check Results2 new issue(s) introduced by this patch (5769 total). New issues on modified lines:
All issues by check:
See inline comments for details. Comment |
Contributor
Author
|
@phlexbot header-guards-fix |
Contributor
|
Automatic header-guards fixes pushed (commit ea7dff7). |
Contributor
Clang-Tidy Check Results2 new issue(s) introduced by this patch (5767 total). New issues on modified lines:
All issues by check:
See inline comments for details. Comment |
beojan
reviewed
Jun 3, 2026
Contributor
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/python/src/modulewrap.cpp (1)
298-327: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRelease the
PySequence_Fastresult invalidate_input.
PySequence_Fastreturns a new reference, but this function never decrefscoll;validate_outputhandles the same pattern correctly. Transform/observe registration now leaks one sequence object per call.Suggested fix
if (PyErr_Occurred()) cargs.clear(); // error handled through Python + Py_DECREF(coll); return cargs;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/python/src/modulewrap.cpp` around lines 298 - 327, The validate_input helper leaks the PySequence_Fast result because coll is never released after use. Update validate_input to decref the PySequence_Fast-owned coll before every return path, including the early failure cases and the normal exit, using the same ownership pattern already used in validate_output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/python/src/dyncall.cpp`:
- Around line 106-109: The new local in the argument-count setup inside
dyncall.cpp should follow the repo’s lower_case C++ naming rule. Rename the size
variable used with the unique_ptr allocations in this block from the uppercase
form to a lower_case name, and keep the rest of the logic in the same dyncall
conversion path unchanged.
In `@plugins/python/src/dyncall.hpp`:
- Around line 38-53: The new public alias inside dcarg uses a PascalCase name
that violates the repository’s C++ naming convention. Rename FFIType to a
lower_case identifier, and update its usage for m_value accordingly so dcarg
stays consistent with the rest of the dyncall API.
- Around line 21-24: The header is not self-contained and violates the C++
identifier naming rule: add the missing <string> include so dcarg::from_str can
use std::string without relying on transitive includes, and rename the FFIType
alias to a lower_case name such as ffi_type throughout the dyncall.hpp
declarations and any related uses while keeping the rest of the dcarg API
unchanged.
In `@plugins/python/src/modulewrap.cpp`:
- Line 50: The constexpr constant MAX_SUPPORTED_ARGS in modulewrap.cpp should be
renamed to lower_case to match the C++ identifier convention. Update the
declaration itself to max_supported_args and make sure the two call sites that
compare against or use this limit in the module wrapping logic are also switched
to the new name, keeping the behavior unchanged.
- Around line 1059-1061: The transform registration in modulewrap.cpp indexes
output_suffixes[0] without first confirming there is at least one suffix. Update
the transform setup around the pyname/pyoutput/out_type assignments to validate
output_product_suffixes (and the derived output_suffixes) is non-empty before
any indexing, and reject the registration with a clear error if it is omitted or
empty.
- Around line 599-609: The provider callback conversion in
provider_cb_##name::operator() does not propagate Python conversion failures, so
a bad provider return can leave a pending Python error while still returning a
C++ value. Update the operator() path that calls frompy(pyres) to check
msg_from_py_error (as py_to_* does) and convert any Python error into a failure
before returning; use the existing provider_cb_* and frompy helpers to locate
the fix.
- Around line 740-741: The transform call in modulewrap.cpp is hardcoding the
converter concurrency instead of using the parsed/default setting. Update the
ph_module->transform invocation to remove the explicit casted value and keep the
converter path serial unless a valid concurrency enum/API value is intentionally
supplied. Use the transform call site and the concurrency argument handling
around name and converter to locate and fix it.
- Around line 128-156: The copy/assignment logic in py_callback_base is still
using Py_INCREF/Py_DECREF directly, which can crash when m_callable has been
cleared by move construction or move assignment. Update the copy constructor,
copy assignment operator, and move assignment operator in py_callback_base to
use null-safe reference-count handling with Py_XINCREF/Py_XDECREF (or equivalent
guarded checks) so assigning from or into a moved-from instance is safe. Keep
the existing GIL protection around all reference-count changes.
In `@test/python/CMakeLists.txt`:
- Around line 291-301: The `py:phlex` test is being configured before
`PYTHON_TEST_FILES` includes `test_phlex.py`, so the test may not be generated
or may fail when `set_property(TEST py:phlex ...)` runs. Move the
`set(PYTHON_TEST_FILES ${PYTHON_TEST_FILES} test_phlex.py)` update in the
`HAS_CPPYY` block to before the test-generation step, keeping the `py:phlex`
setup and `PYTHON_TEST_FILES` mutation aligned.
In `@test/python/jited.py`:
- Line 8: Replace the internal numba.core.decorators import in jited.py with
Numba’s public cfunc API so the test no longer depends on unstable internals.
Update the import and any references that use nb_dec so they call cfunc
directly, keeping the test aligned with the supported public symbol and
resilient across Numba versions.
In `@test/python/pyjited.jsonnet`:
- Around line 13-16: The pyjited module stanza is missing the input wiring
needed for the transforms registered in jited.py, so the Numba path never gets
exercised. Update the modules entry for pyadd in pyjited.jsonnet to include the
same kind of input list used by pyadd.jsonnet, ensuring cppsource4py products
are connected for the suffixes consumed by jited.py (such as i, j, u1, and u2).
Keep the fix centered on the pyadd module definition and the jited.py transform
expectations.
---
Outside diff comments:
In `@plugins/python/src/modulewrap.cpp`:
- Around line 298-327: The validate_input helper leaks the PySequence_Fast
result because coll is never released after use. Update validate_input to decref
the PySequence_Fast-owned coll before every return path, including the early
failure cases and the normal exit, using the same ownership pattern already used
in validate_output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d890ab78-b06b-42ff-ab15-69f594e3c6f5
📒 Files selected for processing (8)
plugins/python/CMakeLists.txtplugins/python/python/phlex/_typing.pyplugins/python/src/dyncall.cppplugins/python/src/dyncall.hppplugins/python/src/modulewrap.cpptest/python/CMakeLists.txttest/python/jited.pytest/python/pyjited.jsonnet
📜 Review details
⚠️ CI failures not shown inline (9)
GitHub Actions: wlav checking C++ code format / 0_clang-format-check.txt: wlav checking C++ code format
Conclusion: failure
##[group]Run REPO_NAME="${REPO##*/}"
�[36;1mREPO_NAME="${REPO##*/}"�[0m
�[36;1mif [ "success" = 'success' ]; then�[0m
�[36;1m echo "✅ clang-format check passed."�[0m
�[36;1melse�[0m
�[36;1m echo "::error::clang-format check failed. Please review the output above for details."�[0m
GitHub Actions: wlav checking C++ code with clang-tidy / 1_clang-tidy-check.txt: wlav checking C++ code with clang-tidy
Conclusion: failure
##[group]Run . /entrypoint.sh
�[36;1m. /entrypoint.sh�[0m
�[36;1mREPO_NAME="${REPO##*/}"�[0m
�[36;1mSOURCE_DIR="$GITHUB_WORKSPACE/$SOURCE_PATH"�[0m
�[36;1mBUILD_DIR="$GITHUB_WORKSPACE/$BUILD_PATH"�[0m
�[36;1m�[0m
�[36;1m# run-clang-tidy runs clang-tidy on every translation unit in�[0m
�[36;1m# compile_commands.json in parallel and merges the per-TU fix YAML files�[0m
�[36;1m# into a single comprehensive clang-tidy-fixes.yaml. The alternative�[0m
�[36;1m# (setting CMAKE_CXX_CLANG_TIDY=clang-tidy;--export-fixes=... and�[0m
�[36;1m# building with -j) suffers from a race condition: every parallel�[0m
�[36;1m# clang-tidy invocation independently overwrites the same output file, so�[0m
�[36;1m# only the fixes from whichever translation unit finishes last are�[0m
�[36;1m# retained.�[0m
�[36;1m#�[0m
�[36;1m# Path arguments are substring-matched against file paths in�[0m
�[36;1m# compile_commands.json; restricting to the source directory automatically�[0m
�[36;1m# excludes generated files (e.g. version.cpp, ROOT dictionaries) that live�[0m
�[36;1m# in the build directory and lack access to the .clang-tidy config.�[0m
�[36;1m#�[0m
�[36;1m# run-clang-tidy validates the check list by calling clang-tidy from its�[0m
�[36;1m# working directory; run from the source root so that clang-tidy discovers�[0m
�[36;1m# the .clang-tidy config file there, preventing a spurious "No checks�[0m
�[36;1m# enabled." error.�[0m
�[36;1mif ! command -v run-clang-tidy >/dev/null 2>&1; then�[0m
�[36;1m echo "::error::run-clang-tidy not found in PATH; cannot run clang-tidy checks"�[0m
GitHub Actions: wlav checking Python code / 0_python-check.txt: wlav checking Python code
Conclusion: failure
##[group]Run REPO_NAME="${REPO##*/}"
�[36;1mREPO_NAME="${REPO##*/}"�[0m
�[36;1mif [ "success" = 'success' ] && [ "success" = 'success' ]; then�[0m
�[36;1m echo "✅ Python checks passed."�[0m
�[36;1melse�[0m
�[36;1m echo "::error::Python checks failed. Comment '@${REPO_NAME}bot python-fix' on the PR to attempt auto-fix."�[0m
GitHub Actions: wlav checking Python code / 1_scripts-test.txt: wlav checking Python code
Conclusion: failure
test_nothing_written_when_no_alerts PASSED [ 23%]
scripts/test/test_check_codeql_alerts.py::TestMainSarifMode::test_new_alert_returns_zero_and_writes_comment PASSED [ 23%]
scripts/test/test_check_codeql_alerts.py::TestMainSarifMode::test_absent_alert_returns_zero_and_writes_comment PASSED [ 24%]
scripts/test/test_check_codeql_alerts.py::TestMainSarifMode::test_no_alerts_returns_zero_no_comment PASSED [ 24%]
scripts/test/test_check_codeql_alerts.py::TestMainSarifMode::test_below_threshold_alert_not_reported PASSED [ 24%]
scripts/test/test_check_codeql_alerts.py::TestMainSarifMode::test_missing_sarif_exits_nonzero PASSED [ 24%]
scripts/test/test_check_codeql_alerts.py::TestMainSarifMode::test_github_output_written PASSED [ 24%]
scripts/test/test_check_codeql_alerts.py::TestMainSarifMode::test_github_output_false_when_no_alerts PASSED [ 25%]
scripts/test/test_check_codeql_alerts.py::TestMainSarifMode::test_directory_of_sarif_files PASSED [ 25%]
scripts/test/test_check_codeql_alerts.py::TestMainSarifMode::test_sarif_mode_pr_ref_produces_filtered_url PASSED [ 25%]
scripts/test/test_check_codeql_alerts.py::TestMainSarifMode::test_non_integer_pr_ref_no_filtered_url PASSED [ 25%]
scripts/test/test_check_codeql_alerts.py::TestMainApiMode::test_api_mode_new_alert PASSED [ 25%]
scripts/test/test_check_codeql_alerts.py::TestMainApiMode::test_api_mode_min_level_filtering PASSED [ 26%]
scripts/test/test_check_codeql_alerts.py::TestMainApiMode::test_api_mode_github_api_error_exits_2 PASSED [ 26%]
scripts/test/test_check_codeql_alerts.py::TestMainApiMode::test_api_mode_missing_github_repository_exits_2 PASSED [ 26%]
scripts/test/test_check_codeql_alerts.py::TestMainApiMode::test_api_mode_skipped_when_sarif_has_baseline PASSED [ 26%]
scripts/test/test_check_codeql_alerts.py::TestMainApiModeWithPrRef::test_api_mode_pr_ref_produces_filtered_url PASSED [ 27%]
scripts/test/test_check_codeql_alerts.py::TestMainEntrypoint::test_entrypoint_no_alerts_exits_zero PASSED [ ...
GitHub Actions: wlav checking Jsonnet format / 0_jsonnet-format-check.txt: wlav checking Jsonnet format
Conclusion: failure
##[group]Run REPO_NAME="${REPO##*/}"
�[36;1mREPO_NAME="${REPO##*/}"�[0m
�[36;1mif [ "success" = 'success' ]; then�[0m
�[36;1m echo "✅ Jsonnet formatting check passed."�[0m
�[36;1melse�[0m
�[36;1m echo "::error::Jsonnet formatting issues found. Please review the output above for details."�[0m
GitHub Actions: wlav building and testing Framework-R-D/phlex / 1_build (gcc, none).txt: wlav building and testing Framework-R-D/phlex
Conclusion: failure
�[36;1mecho "::group::Running ctest"�[0m
�[36;1mif ctest --progress --output-on-failure -j "$(nproc)"; then�[0m
�[36;1m echo "::endgroup::"�[0m
�[36;1m echo "✅ All tests passed."�[0m
�[36;1melse�[0m
�[36;1m echo "::endgroup::"�[0m
�[36;1m echo "::error:: Some tests failed."�[0m
GitHub Actions: wlav running code coverage / 2_coverage.txt: wlav running code coverage
Conclusion: failure
�[36;1mecho "::group::Running ctest for coverage"�[0m
�[36;1mif ctest --progress --output-on-failure -j "$(nproc)"; then�[0m
�[36;1m echo "::endgroup::"�[0m
�[36;1m echo "✅ All tests passed."�[0m
�[36;1melse�[0m
�[36;1m echo "::endgroup::"�[0m
�[36;1m echo "::error::Some tests failed."�[0m
GitHub Actions: wlav checking CMake format / 0_cmake-format-check.txt: wlav checking CMake format
Conclusion: failure
##[group]Run REPO_NAME="${REPO##*/}"
�[36;1mREPO_NAME="${REPO##*/}"�[0m
�[36;1mif [ "success" = 'success' ]; then�[0m
�[36;1m echo "✅ All CMake files are properly formatted."�[0m
�[36;1melse�[0m
�[36;1m echo "::error::Found files with formatting issues."�[0m
GitHub Actions: CodeQL Analysis / 3_determine-languages.txt: Initial Numba support
Conclusion: failure
##[group]Run # If detection was skipped, use all languages or the provided language-matrix
�[36;1m# If detection was skipped, use all languages or the provided language-matrix�[0m
�[36;1mif [ "${SKIP_DETECTION}" = "true" ]; then�[0m
�[36;1m if [ "${GITHUB_EVENT_NAME}" = "workflow_call" ] && [ -n "${LANGUAGE_MATRIX}" ]; then�[0m
�[36;1m # Validate that language-matrix is valid JSON�[0m
�[36;1m if ! echo "${LANGUAGE_MATRIX}" | python3 -c "import sys, json; json.load(sys.stdin)" 2>/dev/null; then�[0m
�[36;1m echo "::error::Invalid language-matrix input: must be valid JSON array"�[0m
🧰 Additional context used
📓 Path-based instructions (7)
**/*.jsonnet
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
jsonnetfmtfor consistent Jsonnet formatting; CI enforces thisUse
jsonnetfmtformatting for Jsonnet files; CI enforces this
Files:
test/python/pyjited.jsonnet
**/*.{cpp,cc,cxx,h,hpp}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{cpp,cc,cxx,h,hpp}: Use clang-format tool for all C++ code formatting (VS Code auto-formats on save); configuration defined in.clang-formatwith 100-character line limit and 2-space indentation
Follow clang-tidy recommendations defined in.clang-tidy
Files:
plugins/python/src/dyncall.cppplugins/python/src/dyncall.hppplugins/python/src/modulewrap.cpp
**/*.{hpp,cpp}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{hpp,cpp}: Use.hppfor header files,.cppfor implementation, and*_test.cppfor test files in C++
Enforce 100-character line limit and 2-space indentation in C++ code via.clang-format
UseQualifierAlignment: Right(east-const) style:int const xnotconst int xin C++
UsePointerAlignment: Leftin C++ (pointer*attached to type, not variable name)
All C++ identifiers must uselower_casenaming: namespaces, classes, structs, enums, functions, variables, parameters, members, and constants
Exception to C++ naming: template parameters useCamelCase
Exception to C++ naming: macros useUPPER_CASE
Private, protected, and constant members in C++ must have a trailing underscore (_), no trailing underscore on anything else
Useenum classpreferred over plainenumin C++
Usestd::shared_ptrfor shared ownership,std::unique_ptrfor exclusive ownership, raw pointers for non-owning references only in C++
Use functors with agent-noun pattern:ModelEvaluator evaluate_model(...)in C++
Apply.clang-tidychecks for bugprone, cert, clang-analyzer, concurrency, cppcoreguidelines, misc, modernize, performance, portability, and readability as defined in the.clang-tidyconfiguration file
Usephlex::namespace for core code,phlex::experimental::for experimental features in C++
Files:
plugins/python/src/dyncall.cppplugins/python/src/dyncall.hppplugins/python/src/modulewrap.cpp
plugins/python/**/*.cpp
📄 CodeRabbit inference engine (AGENTS.md)
plugins/python/**/*.cpp: Usestd::runtime_errorfor C++ runtime failures; propagate Python exceptions viaPyErr_SetString/PyErr_Format; returnnullptron error; callPyErr_Clear()when recovering in Python/C++ integration
Use manualPy_INCREF/Py_DECREFfor reference counting andPyGILRAIIRAII wrapper for GIL management in C++ code that interacts with Python
For GC-tracked Python types in C++: usePy_TPFLAGS_HAVE_GC, implementtp_traverseandtp_clear, callPyObject_GC_UnTrackbefore deallocation
Files:
plugins/python/src/dyncall.cppplugins/python/src/modulewrap.cpp
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Use ruff for Python formatting and linting (configured inpyproject.toml); follow Google-style docstring conventions; use line length of 99 characters; use double quotes for strings
Usefrom __future__ import annotationsto enable deferred evaluation of type annotations (avoids forward-reference issues; Python >=3.12)
Type hints are recommended; mypy is configured inpyproject.toml
Avoid naming Python test scriptstypes.pyor other names that shadow standard library modules, as this causes obscure import errors
Python test scripts should follow the test structure pattern: C++ driver provides data streams, Jsonnet config wires the graph, and Python script implements algorithms
**/*.py: Enforce 99-character line limit and double quotes in Python via ruff configured inpyproject.toml
Use Google-style docstrings in Python code
Use type hints in Python code and configure mypy for type checking
Usefrom __future__ import annotationsin Python to enable deferred evaluation of type annotations
Use PEP 8 naming in Python:CapWordsfor classes,snake_casefor everything else
When using thereadtool for Python files, always use integer values foroffsetandlimitparameters, never float/double values
Files:
test/python/jited.pyplugins/python/python/phlex/_typing.py
test/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Name Python test files as
test_*.py, do NOT name files after stdlib modules (e.g.,types.py)
Files:
test/python/jited.py
**/*.hpp
📄 CodeRabbit inference engine (AGENTS.md)
Avoid boolean parameters in C++ interfaces; prefer enumerations instead
Files:
plugins/python/src/dyncall.hpp
🧠 Learnings (1)
📚 Learning: 2026-06-18T23:48:04.336Z
Learnt from: wlav
Repo: Framework-R-D/phlex PR: 626
File: test/python/jited.py:25-25
Timestamp: 2026-06-18T23:48:04.336Z
Learning: In Phlex Python test/module files under `test/python/` (e.g., `test/python/jited.py`), treat `PHLEX_REGISTER_ALGORITHMS` as an intentional macro-based interface that mirrors the equivalent C++ macro. Do not add or change type hints for `PHLEX_REGISTER_ALGORITHMS`; it should remain untyped because it’s part of Phlex’s macro contract and Phlex internal types are not exposed to Python by design.
Applied to files:
test/python/jited.py
🪛 Cppcheck (2.21.0)
plugins/python/src/dyncall.cpp
[style] 15-15: The function 'from_str' is never used.
(unusedFunction)
[style] 96-96: The function 'dyncall' is never used.
(unusedFunction)
🔇 Additional comments (2)
plugins/python/python/phlex/_typing.py (1)
17-28: LGTM!Also applies to: 50-66, 123-127
plugins/python/CMakeLists.txt (1)
10-12: LGTM!Also applies to: 23-29
Contributor
|
@wlav A de novo review by CodeRabbit claims to have identified several correctness issues. Ignoring the naming convention complaints, could you take a look, including at the reference-counting issue in the review walkthrough rather than a diff-specific comment thread? |
…d first" This reverts commit 4c4741b.
Contributor
Author
|
@greenc-FNAL: done. Mostly good stuff actually. |
Contributor
|
@coderabbitai review Ignore/resolve outstanding naming comments: naming issues will be addressed en masse after the upcoming 0.3 release. |
Contributor
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
owner="Framework-R-D"
repo="phlex"
pr="626"
gh api graphql \
-F owner="$owner" \
-F name="$repo" \
-F pr="$pr" \
-f query='
query($owner: String!, $name: String!, $pr: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $pr) {
reviewThreads(first: 100) {
nodes {
id
isResolved
isOutdated
path
line
comments(first: 20) {
nodes {
author { login }
body
url
createdAt
}
}
}
}
}
}
}' > /tmp/pr_threads.json
python - <<'PY'
import json
from pathlib import Path
data = json.loads(Path('/tmp/pr_threads.json').read_text())
threads = data["data"]["repository"]["pullRequest"]["reviewThreads"]["nodes"]
for i, t in enumerate(threads, 1):
if t["isResolved"]:
continue
authors = {c["author"]["login"] if c["author"] else "UNKNOWN" for c in t["comments"]["nodes"]}
if not any(a.lower().startswith("coderabbit") for a in authors):
continue
print("=" * 100)
print(f"THREAD {i}")
print(f"id : {t['id']}")
print(f"path : {t['path']}")
print(f"line : {t['line']}")
print(f"resolved : {t['isResolved']}")
print(f"outdated : {t['isOutdated']}")
for j, c in enumerate(t["comments"]["nodes"], 1):
author = c["author"]["login"] if c["author"] else "UNKNOWN"
body = c["body"].strip().replace("\r", "")
print(f"\n COMMENT {j}")
print(f" author : {author}")
print(f" url : {c['url']}")
print(" body:")
for line in body.splitlines():
print(f" {line}")
PYLength of output: 1868 🏁 Script executed: #!/bin/bash
set -euo pipefail
owner="Framework-R-D"
repo="phlex"
pr="626"
gh api graphql \
-F owner="$owner" \
-F name="$repo" \
-F pr="$pr" \
-f query='
query($owner: String!, $name: String!, $pr: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $pr) {
reviewThreads(first: 100) {
nodes {
id
isResolved
isOutdated
path
line
comments(first: 10) {
nodes {
author { login }
body
url
}
}
}
}
}
}
}' | python - <<'PY'
import json, sys
data = json.load(sys.stdin)
threads = data["data"]["repository"]["pullRequest"]["reviewThreads"]["nodes"]
found = 0
for t in threads:
if t["isResolved"]:
continue
comments = t["comments"]["nodes"]
authors = [(c["author"]["login"] if c["author"] else "UNKNOWN") for c in comments]
if not any(a.lower().startswith("coderabbit") for a in authors):
continue
found += 1
print("---")
print(f"thread_id: {t['id']}")
print(f"path: {t['path']}")
print(f"line: {t['line']}")
print(f"outdated: {t['isOutdated']}")
for idx, c in enumerate(comments, 1):
author = c["author"]["login"] if c["author"] else "UNKNOWN"
body = " ".join(c["body"].split())
if len(body) > 300:
body = body[:300] + "..."
print(f"comment_{idx}_author: {author}")
print(f"comment_{idx}_url: {c['url']}")
print(f"comment_{idx}_body: {body}")
if found == 0:
print("NO_UNRESOLVED_CODERABBIT_THREADS")
PYLength of output: 2682 🏁 Script executed: #!/bin/bash
set -euo pipefail
owner="Framework-R-D"
repo="phlex"
pr="626"
echo "== gh version =="
gh --version || true
echo
echo "== gh auth status =="
gh auth status || true
echo
echo "== graphql raw query =="
set +e
gh api graphql \
-F owner="$owner" \
-F name="$repo" \
-F pr="$pr" \
-f query='
query($owner: String!, $name: String!, $pr: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $pr) {
reviewThreads(first: 20) {
nodes {
id
isResolved
path
line
}
}
}
}
}' > /tmp/gh_graphql_stdout.txt 2> /tmp/gh_graphql_stderr.txt
status=$?
set -e
echo "graphql_exit_status=$status"
echo "--- graphql stdout ---"
cat /tmp/gh_graphql_stdout.txt || true
echo
echo "--- graphql stderr ---"
cat /tmp/gh_graphql_stderr.txt || true
echo
echo "== gh pr view fallback =="
set +e
gh pr view "https://github.com/${owner}/${repo}/pull/${pr}" --json number,title,reviewDecision,reviews,comments > /tmp/gh_pr_view.json 2> /tmp/gh_pr_view_stderr.txt
status2=$?
set -e
echo "pr_view_exit_status=$status2"
echo "--- pr view stderr ---"
cat /tmp/gh_pr_view_stderr.txt || true
echo "--- pr view summary ---"
python - <<'PY'
import json
from pathlib import Path
p = Path('/tmp/gh_pr_view.json')
if not p.exists() or not p.read_text().strip():
print("NO_PR_VIEW_JSON")
else:
data = json.loads(p.read_text())
print("number:", data.get("number"))
print("title:", data.get("title"))
print("reviewDecision:", data.get("reviewDecision"))
print("reviews:", len(data.get("reviews", [])))
print("comments:", len(data.get("comments", [])))
for c in data.get("comments", [])[:10]:
author = (c.get("author") or {}).get("login")
body = " ".join((c.get("body") or "").split())
print(f"- author={author} body={body[:220]}")
PYLength of output: 7079
I’ll treat naming nits as deferred for now and focus this pass on substantive issues only: correctness, safety, refcounting/lifetime, API behavior, and test/build-impact. ✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
plugins/python/src/modulewrap.cpp (3)
681-690: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate list element conversion errors instead of returning partial vectors.
The list branch pushes the converted value, clears any Python error, breaks, and then returns the partially-filled vector. A single bad element can silently corrupt the graph data.
Suggested fix
Py_ssize_t total = PyList_Size(pyobj); vec->reserve(total); for (Py_ssize_t i = 0; i < total; ++i) { PyObject* item = PyList_GetItem(pyobj, i); - vec->push_back(static_cast<cpptype>(frompy(item))); - if (PyErr_Occurred()) { - PyErr_Clear(); - break; + cpptype value = static_cast<cpptype>(frompy(item)); + std::string msg; + if (msg_from_py_error(msg, true)) { + Py_DECREF(pyobj); + throw std::runtime_error("List conversion error for type " `#name` ": " + msg); } + vec->push_back(value); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/python/src/modulewrap.cpp` around lines 681 - 690, The PyList_Check branch in modulewrap.cpp is swallowing element conversion failures and returning a partially filled vector, so update this path to stop on the first bad item and propagate the Python error instead of clearing it. In the list conversion loop, use frompy(item) only after confirming the conversion succeeded, and if PyErr_Occurred() becomes true, do not continue building vec; let the error bubble out from the list-handling logic that wraps PyList_Check.
421-426: 🩺 Stability & Availability | 🟠 Majorplugins/python/src/modulewrap.cpp:421-479 — Clear the missing-
__annotations__error before falling back to_sig.
PyObject_GetAttr(callable, sann)leavesAttributeErrorpending when__annotations__is absent. If the Numba_sigpath succeeds,annotations_to_strings()still returnstruewith that exception set, which can surface later as a CPython failure. Clear the missing-attribute error before trying_sig, and keep"unknown annotation formatting"for the remaining failure cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/python/src/modulewrap.cpp` around lines 421 - 426, The fallback logic in modulewrap.cpp’s annotation handling leaves a pending AttributeError after PyObject_GetAttr(callable, sann) fails, which can leak through even when the _sig path succeeds. Update the annotations_to_strings flow to clear the missing-__annotations__ error before attempting the Numba _sig fallback, and keep the existing "unknown annotation formatting" message only for the cases where both annotation lookup and signature fallback fail.
617-631: 🎯 Functional Correctness | 🟠 MajorHandle null vector conversion without returning NULL.
PyObject_CallFunctionObjArgstreatsNULLas the end of the variadic list, sodcarg{nullptr}will truncate the Python call; it can also lead toPy_DECREF(nullptr)indecref_all(). UsePy_Nonefor intentional empties and throw onPyArray_SimpleNewFromData/lifeline allocation failures.plugins/python/src/modulewrap.cpp:617-631🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/python/src/modulewrap.cpp` around lines 617 - 631, The null-vector path in the conversion helper returns dcarg{nullptr}, which can be misread by PyObject_CallFunctionObjArgs and later trigger decref issues in decref_all(). Update the conversion logic in modulewrap.cpp’s numpy view creation path so intentional empty values are represented with Py_None instead of NULL, and make PyArray_SimpleNewFromData and any lifeline allocation failures raise an exception rather than returning a null argument wrapper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/python/src/modulewrap.cpp`:
- Line 1048: The JIT callback wiring in insert_input_converters and the related
output setup still allows collection annotations to reach the libffi/Numba path,
which can pass PyObject* values into a cfunc before converters reject them. Add
an early guard in the JIT path, before callbacks are wired, to reject collection
types such as ndarray and list for both inputs and outputs, and make sure
jit_callback and the input/output converter setup bail out before calling
dcarg::from_str or selecting v*_to_py converters for these annotations.
In `@test/python/adder.py`:
- Around line 59-64: The negative-path check in the test for m.transform should
explicitly fail if no TypeError is raised, since it currently only validates the
exception message and can still pass when the missing-output-suffix validation
regresses. Update the try/except around m.transform(int_adder,
input_family=config["input"]) to include an else branch that raises an assertion
failure, keeping the existing TypeError message check intact. Use the
m.transform call and the TypeError handler in adder.py as the anchor points for
the fix.
---
Outside diff comments:
In `@plugins/python/src/modulewrap.cpp`:
- Around line 681-690: The PyList_Check branch in modulewrap.cpp is swallowing
element conversion failures and returning a partially filled vector, so update
this path to stop on the first bad item and propagate the Python error instead
of clearing it. In the list conversion loop, use frompy(item) only after
confirming the conversion succeeded, and if PyErr_Occurred() becomes true, do
not continue building vec; let the error bubble out from the list-handling logic
that wraps PyList_Check.
- Around line 421-426: The fallback logic in modulewrap.cpp’s annotation
handling leaves a pending AttributeError after PyObject_GetAttr(callable, sann)
fails, which can leak through even when the _sig path succeeds. Update the
annotations_to_strings flow to clear the missing-__annotations__ error before
attempting the Numba _sig fallback, and keep the existing "unknown annotation
formatting" message only for the cases where both annotation lookup and
signature fallback fail.
- Around line 617-631: The null-vector path in the conversion helper returns
dcarg{nullptr}, which can be misread by PyObject_CallFunctionObjArgs and later
trigger decref issues in decref_all(). Update the conversion logic in
modulewrap.cpp’s numpy view creation path so intentional empty values are
represented with Py_None instead of NULL, and make PyArray_SimpleNewFromData and
any lifeline allocation failures raise an exception rather than returning a null
argument wrapper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ae868e6a-cefb-495d-9d83-6405f2a303e1
📒 Files selected for processing (6)
plugins/python/src/dyncall.cppplugins/python/src/dyncall.hppplugins/python/src/modulewrap.cpptest/python/CMakeLists.txttest/python/adder.pytest/python/jited.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: clang-tidy-check
⚠️ CI failures not shown inline (17)
GitHub Actions: wlav checking CMake format / 0_cmake-format-check.txt: wlav checking CMake format
Conclusion: failure
##[group]Run REPO_NAME="${REPO##*/}"
�[36;1mREPO_NAME="${REPO##*/}"�[0m
�[36;1mif [ "success" = 'success' ]; then�[0m
�[36;1m echo "✅ All CMake files are properly formatted."�[0m
�[36;1melse�[0m
�[36;1m echo "::error::Found files with formatting issues."�[0m
GitHub Actions: wlav checking CMake format / cmake-format-check: wlav checking CMake format
Conclusion: failure
##[group]Run REPO_NAME="${REPO##*/}"
�[36;1mREPO_NAME="${REPO##*/}"�[0m
�[36;1mif [ "success" = 'success' ]; then�[0m
�[36;1m echo "✅ All CMake files are properly formatted."�[0m
�[36;1melse�[0m
�[36;1m echo "::error::Found files with formatting issues."�[0m
GitHub Actions: wlav checking Jsonnet format / jsonnet-format-check: wlav checking Jsonnet format
Conclusion: failure
##[group]Run REPO_NAME="${REPO##*/}"
�[36;1mREPO_NAME="${REPO##*/}"�[0m
�[36;1mif [ "success" = 'success' ]; then�[0m
�[36;1m echo "✅ Jsonnet formatting check passed."�[0m
�[36;1melse�[0m
�[36;1m echo "::error::Jsonnet formatting issues found. Please review the output above for details."�[0m
GitHub Actions: wlav checking C++ code format / 0_clang-format-check.txt: wlav checking C++ code format
Conclusion: failure
##[group]Run REPO_NAME="${REPO##*/}"
�[36;1mREPO_NAME="${REPO##*/}"�[0m
�[36;1mif [ "success" = 'success' ]; then�[0m
�[36;1m echo "✅ clang-format check passed."�[0m
�[36;1melse�[0m
�[36;1m echo "::error::clang-format check failed. Please review the output above for details."�[0m
GitHub Actions: wlav checking C++ code format / clang-format-check: wlav checking C++ code format
Conclusion: failure
##[group]Run REPO_NAME="${REPO##*/}"
�[36;1mREPO_NAME="${REPO##*/}"�[0m
�[36;1mif [ "success" = 'success' ]; then�[0m
�[36;1m echo "✅ clang-format check passed."�[0m
�[36;1melse�[0m
�[36;1m echo "::error::clang-format check failed. Please review the output above for details."�[0m
GitHub Actions: wlav checking Jsonnet format / 0_jsonnet-format-check.txt: wlav checking Jsonnet format
Conclusion: failure
##[group]Run REPO_NAME="${REPO##*/}"
�[36;1mREPO_NAME="${REPO##*/}"�[0m
�[36;1mif [ "success" = 'success' ]; then�[0m
�[36;1m echo "✅ Jsonnet formatting check passed."�[0m
�[36;1melse�[0m
�[36;1m echo "::error::Jsonnet formatting issues found. Please review the output above for details."�[0m
GitHub Actions: wlav checking Python code / scripts-test: wlav checking Python code
Conclusion: failure
##[group]Run codecov/codecov-action@fb8b3582c8e4def4969c97caa2f19720cb33a72f
with:
files: coverage-scripts.xml
flags: scripts
name: phlex-scripts-coverage
fail_ci_if_error: false
verbose: true
root_dir: phlex-src
disable_file_fixes: false
disable_search: false
disable_safe_directory: false
disable_telem: false
dry_run: false
git_service: github
gcov_executable: gcov
handle_no_reports_found: false
recurse_submodules: false
run_command: upload-coverage
skip_validation: false
use_legacy_upload_endpoint: false
use_oidc: false
use_pypi: false
version: latest
env:
CODECOV_***REDACTED*** /home/runner/work/_temp/uv-python-dir
UV_CACHE_DIR: /home/runner/work/_temp/setup-uv-cache
##[endgroup]
##[group]Run missing_deps=""
�[36;1mmissing_deps=""�[0m
�[36;1m�[0m
�[36;1m# Check for always-required commands�[0m
�[36;1mfor cmd in bash git curl; do�[0m
�[36;1m if ! command -v "$cmd" >/dev/null 2>&1; then�[0m
�[36;1m missing_deps="$missing_deps $cmd"�[0m
�[36;1m fi�[0m
�[36;1mdone�[0m
�[36;1m�[0m
�[36;1m# Check for gpg only if validation is not being skipped�[0m
�[36;1mif [ "$INPUT_SKIP_VALIDATION" != "true" ]; then�[0m
�[36;1m if ! command -v gpg >/dev/null 2>&1; then�[0m
�[36;1m missing_deps="$missing_deps gpg"�[0m
�[36;1m fi�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1m# Report missing required dependencies�[0m
�[36;1mif [ -n "$missing_deps" ]; then�[0m
�[36;1m echo "Error: The following required dependencies are missing:$missing_deps"�[0m
�[36;1m echo "Please install these dependencies before using this action."�[0m
�[36;1m exit 1�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mecho "All required system dependencies are available."�[0m
shell: /usr/bin/sh -e {0}
env:
CODECOV_***REDACTED*** /home/runner/work/_temp/uv-python-dir
UV_CACHE_DIR: /home/runner/work/_temp/setup-uv-cache
INPUT_SKIP_VALIDATION: false
##[endgroup]
All required system dependencies are available.
##[grou...
GitHub Actions: wlav checking Python code / python-check: wlav checking Python code
Conclusion: failure
##[group]Run REPO_NAME="${REPO##*/}"
�[36;1mREPO_NAME="${REPO##*/}"�[0m
�[36;1mif [ "success" = 'success' ] && [ "success" = 'success' ]; then�[0m
�[36;1m echo "✅ Python checks passed."�[0m
�[36;1melse�[0m
�[36;1m echo "::error::Python checks failed. Comment '@${REPO_NAME}bot python-fix' on the PR to attempt auto-fix."�[0m
GitHub Actions: wlav checking Python code / 1_python-check.txt: wlav checking Python code
Conclusion: failure
##[group]Run REPO_NAME="${REPO##*/}"
�[36;1mREPO_NAME="${REPO##*/}"�[0m
�[36;1mif [ "success" = 'success' ] && [ "success" = 'success' ]; then�[0m
�[36;1m echo "✅ Python checks passed."�[0m
�[36;1melse�[0m
�[36;1m echo "::error::Python checks failed. Comment '@${REPO_NAME}bot python-fix' on the PR to attempt auto-fix."�[0m
GitHub Actions: wlav checking Python code / 0_scripts-test.txt: wlav checking Python code
Conclusion: failure
estMainSarifMode::test_new_alert_returns_zero_and_writes_comment PASSED [ 23%]
scripts/test/test_check_codeql_alerts.py::TestMainSarifMode::test_absent_alert_returns_zero_and_writes_comment PASSED [ 24%]
scripts/test/test_check_codeql_alerts.py::TestMainSarifMode::test_no_alerts_returns_zero_no_comment PASSED [ 24%]
scripts/test/test_check_codeql_alerts.py::TestMainSarifMode::test_below_threshold_alert_not_reported PASSED [ 24%]
scripts/test/test_check_codeql_alerts.py::TestMainSarifMode::test_missing_sarif_exits_nonzero PASSED [ 24%]
scripts/test/test_check_codeql_alerts.py::TestMainSarifMode::test_github_output_written PASSED [ 24%]
scripts/test/test_check_codeql_alerts.py::TestMainSarifMode::test_github_output_false_when_no_alerts PASSED [ 25%]
scripts/test/test_check_codeql_alerts.py::TestMainSarifMode::test_directory_of_sarif_files PASSED [ 25%]
scripts/test/test_check_codeql_alerts.py::TestMainSarifMode::test_sarif_mode_pr_ref_produces_filtered_url PASSED [ 25%]
scripts/test/test_check_codeql_alerts.py::TestMainSarifMode::test_non_integer_pr_ref_no_filtered_url PASSED [ 25%]
scripts/test/test_check_codeql_alerts.py::TestMainApiMode::test_api_mode_new_alert PASSED [ 25%]
scripts/test/test_check_codeql_alerts.py::TestMainApiMode::test_api_mode_min_level_filtering PASSED [ 26%]
scripts/test/test_check_codeql_alerts.py::TestMainApiMode::test_api_mode_github_api_error_exits_2 PASSED [ 26%]
scripts/test/test_check_codeql_alerts.py::TestMainApiMode::test_api_mode_missing_github_repository_exits_2 PASSED [ 26%]
scripts/test/test_check_codeql_alerts.py::TestMainApiMode::test_api_mode_skipped_when_sarif_has_baseline PASSED [ 26%]
scripts/test/test_check_codeql_alerts.py::TestMainApiModeWithPrRef::test_api_mode_pr_ref_produces_filtered_url PASSED [ 27%]
scripts/test/test_check_codeql_alerts.py::TestMainEntrypoint::test_entrypoint_no_alerts_exits_zero PASSED [ 27%]
scripts/test/test_clang_tidy_check_summary.py::TestLoadDiagnostics::test_reads_from_file...
GitHub Actions: wlav building and testing Framework-R-D/phlex / build (gcc, none): wlav building and testing Framework-R-D/phlex
Conclusion: failure
�[36;1mecho "::group::Running ctest"�[0m
�[36;1mif ctest --progress --output-on-failure -j "$(nproc)"; then�[0m
�[36;1m echo "::endgroup::"�[0m
�[36;1m echo "✅ All tests passed."�[0m
�[36;1melse�[0m
�[36;1m echo "::endgroup::"�[0m
�[36;1m echo "::error:: Some tests failed."�[0m
GitHub Actions: wlav building and testing Framework-R-D/phlex / 1_build (gcc, none).txt: wlav building and testing Framework-R-D/phlex
Conclusion: failure
�[36;1mecho "::group::Running ctest"�[0m
�[36;1mif ctest --progress --output-on-failure -j "$(nproc)"; then�[0m
�[36;1m echo "::endgroup::"�[0m
�[36;1m echo "✅ All tests passed."�[0m
�[36;1melse�[0m
�[36;1m echo "::endgroup::"�[0m
�[36;1m echo "::error:: Some tests failed."�[0m
GitHub Actions: wlav running code coverage / coverage: wlav running code coverage
Conclusion: failure
�[36;1mecho "::group::Running ctest for coverage"�[0m
�[36;1mif ctest --progress --output-on-failure -j "$(nproc)"; then�[0m
�[36;1m echo "::endgroup::"�[0m
�[36;1m echo "✅ All tests passed."�[0m
�[36;1melse�[0m
�[36;1m echo "::endgroup::"�[0m
�[36;1m echo "::error::Some tests failed."�[0m
GitHub Actions: wlav running code coverage / 2_coverage.txt: wlav running code coverage
Conclusion: failure
�[36;1mecho "::group::Running ctest for coverage"�[0m
�[36;1mif ctest --progress --output-on-failure -j "$(nproc)"; then�[0m
�[36;1m echo "::endgroup::"�[0m
�[36;1m echo "✅ All tests passed."�[0m
�[36;1melse�[0m
�[36;1m echo "::endgroup::"�[0m
�[36;1m echo "::error::Some tests failed."�[0m
GitHub Actions: wlav running code coverage / coverage: wlav running code coverage
Conclusion: failure
�[36;1mecho "::group::Running coverage-llvm target"�[0m
�[36;1mif cmake --build . --target coverage-llvm -v; then�[0m
�[36;1m echo "::endgroup::"�[0m
�[36;1m echo "✅ Clang coverage report generation succeeded."�[0m
�[36;1melse�[0m
�[36;1m echo "::endgroup::"�[0m
�[36;1m echo "::error::Clang coverage report generation failed."�[0m
GitHub Actions: CodeQL Analysis / determine-languages: Initial Numba support
Conclusion: failure
##[group]Run # If detection was skipped, use all languages or the provided language-matrix
�[36;1m# If detection was skipped, use all languages or the provided language-matrix�[0m
�[36;1mif [ "${SKIP_DETECTION}" = "true" ]; then�[0m
�[36;1m if [ "${GITHUB_EVENT_NAME}" = "workflow_call" ] && [ -n "${LANGUAGE_MATRIX}" ]; then�[0m
�[36;1m # Validate that language-matrix is valid JSON�[0m
�[36;1m if ! echo "${LANGUAGE_MATRIX}" | python3 -c "import sys, json; json.load(sys.stdin)" 2>/dev/null; then�[0m
�[36;1m echo "::error::Invalid language-matrix input: must be valid JSON array"�[0m
GitHub Actions: CodeQL Analysis / 3_determine-languages.txt: Initial Numba support
Conclusion: failure
##[group]Run # If detection was skipped, use all languages or the provided language-matrix
�[36;1m# If detection was skipped, use all languages or the provided language-matrix�[0m
�[36;1mif [ "${SKIP_DETECTION}" = "true" ]; then�[0m
�[36;1m if [ "${GITHUB_EVENT_NAME}" = "workflow_call" ] && [ -n "${LANGUAGE_MATRIX}" ]; then�[0m
�[36;1m # Validate that language-matrix is valid JSON�[0m
�[36;1m if ! echo "${LANGUAGE_MATRIX}" | python3 -c "import sys, json; json.load(sys.stdin)" 2>/dev/null; then�[0m
�[36;1m echo "::error::Invalid language-matrix input: must be valid JSON array"�[0m
🧰 Additional context used
📓 Path-based instructions (6)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Use ruff for Python formatting and linting (configured inpyproject.toml); follow Google-style docstring conventions; use line length of 99 characters; use double quotes for strings
Usefrom __future__ import annotationsto enable deferred evaluation of type annotations (avoids forward-reference issues; Python >=3.12)
Type hints are recommended; mypy is configured inpyproject.toml
Avoid naming Python test scriptstypes.pyor other names that shadow standard library modules, as this causes obscure import errors
Python test scripts should follow the test structure pattern: C++ driver provides data streams, Jsonnet config wires the graph, and Python script implements algorithms
**/*.py: Enforce 99-character line limit and double quotes in Python via ruff configured inpyproject.toml
Use Google-style docstrings in Python code
Use type hints in Python code and configure mypy for type checking
Usefrom __future__ import annotationsin Python to enable deferred evaluation of type annotations
Use PEP 8 naming in Python:CapWordsfor classes,snake_casefor everything else
When using thereadtool for Python files, always use integer values foroffsetandlimitparameters, never float/double values
Files:
test/python/adder.pytest/python/jited.py
test/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Name Python test files as
test_*.py, do NOT name files after stdlib modules (e.g.,types.py)
Files:
test/python/adder.pytest/python/jited.py
**/*.{cpp,cc,cxx,h,hpp}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{cpp,cc,cxx,h,hpp}: Use clang-format tool for all C++ code formatting (VS Code auto-formats on save); configuration defined in.clang-formatwith 100-character line limit and 2-space indentation
Follow clang-tidy recommendations defined in.clang-tidy
Files:
plugins/python/src/dyncall.hppplugins/python/src/dyncall.cppplugins/python/src/modulewrap.cpp
**/*.{hpp,cpp}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{hpp,cpp}: Use.hppfor header files,.cppfor implementation, and*_test.cppfor test files in C++
Enforce 100-character line limit and 2-space indentation in C++ code via.clang-format
UseQualifierAlignment: Right(east-const) style:int const xnotconst int xin C++
UsePointerAlignment: Leftin C++ (pointer*attached to type, not variable name)
All C++ identifiers must uselower_casenaming: namespaces, classes, structs, enums, functions, variables, parameters, members, and constants
Exception to C++ naming: template parameters useCamelCase
Exception to C++ naming: macros useUPPER_CASE
Private, protected, and constant members in C++ must have a trailing underscore (_), no trailing underscore on anything else
Useenum classpreferred over plainenumin C++
Usestd::shared_ptrfor shared ownership,std::unique_ptrfor exclusive ownership, raw pointers for non-owning references only in C++
Use functors with agent-noun pattern:ModelEvaluator evaluate_model(...)in C++
Apply.clang-tidychecks for bugprone, cert, clang-analyzer, concurrency, cppcoreguidelines, misc, modernize, performance, portability, and readability as defined in the.clang-tidyconfiguration file
Usephlex::namespace for core code,phlex::experimental::for experimental features in C++
Files:
plugins/python/src/dyncall.hppplugins/python/src/dyncall.cppplugins/python/src/modulewrap.cpp
**/*.hpp
📄 CodeRabbit inference engine (AGENTS.md)
Avoid boolean parameters in C++ interfaces; prefer enumerations instead
Files:
plugins/python/src/dyncall.hpp
plugins/python/**/*.cpp
📄 CodeRabbit inference engine (AGENTS.md)
plugins/python/**/*.cpp: Usestd::runtime_errorfor C++ runtime failures; propagate Python exceptions viaPyErr_SetString/PyErr_Format; returnnullptron error; callPyErr_Clear()when recovering in Python/C++ integration
Use manualPy_INCREF/Py_DECREFfor reference counting andPyGILRAIIRAII wrapper for GIL management in C++ code that interacts with Python
For GC-tracked Python types in C++: usePy_TPFLAGS_HAVE_GC, implementtp_traverseandtp_clear, callPyObject_GC_UnTrackbefore deallocation
Files:
plugins/python/src/dyncall.cppplugins/python/src/modulewrap.cpp
🧠 Learnings (1)
📚 Learning: 2026-06-18T23:48:04.336Z
Learnt from: wlav
Repo: Framework-R-D/phlex PR: 626
File: test/python/jited.py:25-25
Timestamp: 2026-06-18T23:48:04.336Z
Learning: In Phlex Python test/module files under `test/python/` (e.g., `test/python/jited.py`), treat `PHLEX_REGISTER_ALGORITHMS` as an intentional macro-based interface that mirrors the equivalent C++ macro. Do not add or change type hints for `PHLEX_REGISTER_ALGORITHMS`; it should remain untyped because it’s part of Phlex’s macro contract and Phlex internal types are not exposed to Python by design.
Applied to files:
test/python/adder.pytest/python/jited.py
🔇 Additional comments (4)
test/python/CMakeLists.txt (1)
88-90: LGTM!test/python/jited.py (1)
8-8: LGTM!Also applies to: 48-56
plugins/python/src/dyncall.hpp (1)
23-23: LGTM!Also applies to: 40-54, 78-82, 87-90, 92-94
plugins/python/src/dyncall.cpp (1)
95-129: LGTM!
Contributor
Clang-Tidy Check ResultsFound 5428 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
Contributor
Clang-Tidy Check ResultsFound 5436 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
greenc-FNAL
approved these changes
Jun 26, 2026
knoepfel
added a commit
to Framework-R-D/phlex-spack-recipes
that referenced
this pull request
Jun 29, 2026
This dependency is required as of Framework-R-D/phlex#626
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds "native" Numba support to phlex.
Numba was usable out-of-the-box already, through Python, but even if the Numba function itself didn't use the GIL, it still needed it at the start of every call (through the wrapper) and all Python/C++ converters required them, too. The implementation in this PR allows for calling the Numba JITed C-function directly, using converters that do not involve Python, and a dispatch through libffi. That sets up a workflow that doesn't require the GIL anywhere beyond configuration/registration time, thus enabling strong scaling (as far as Phlex is concerned) during graph execution.
This PR is for transforms only (observers and providers to follow) and only supports builtin types. Arrays and PODs (by member copy) should be doable as well, but Numba does not support arbitrary C++ classes.
The code in the module wrapper was significantly refactored, to maximize code reuse.
Note: it requires libffi, which will be available on most Linux boxes, but probably needs to be installed with spack in other places, so the spack recipe would need updating as well.
Build system
plugins/python/CMakeLists.txtto locatelibffiviaPkgConfig(pkg_check_modules(FFI ...)usingPkgConfig::FFI) and link it into thepymoduletarget.plugins/python/src/dyncall.cpp(and its companion headerplugins/python/src/dyncall.hpp) to thepymodulebuild.Numba/libffi native call support (core feature)
phlex::experimental::dcarg(type-erasedstd::variantcontainer) andphlex::experimental::dcargs_t.dcarg::from_str()to construct supported argument/return payloads from a fixed set of type-name strings (bool, int/uint variants,ph_long_t/ph_ulong_t, float/double, void/null), throwing on unsupported types.dcarg::value_ptr()plus internal ffi type mapping and aphlex::experimental::dyncall()implementation that buildsffi_cif/argument metadata at runtime and calls viaffi_call.plugins/python/python/phlex/_typing.py:numba.core.typesand extends Python→C++ scalar mapping with Numba scalar types."float32" → "float"and"float64" → "double"handling (alongside existing long long/unsigned long long special cases).Python module wrapper + graph wiring refactor (transforms)
plugins/python/src/modulewrap.cppto maximize code reuse and route dispatch throughdcarg/dyncall:py_callback_base/py_callback_impl/jit_callback_implusingdcarg.CFuncand dispatch selection between Python-callback path vs JIT (Numba) function-pointer path.unroll_switch).nconcur).input_selectors) rather than query-based validation.md_observe/sc_provideto validate selector dictionaries via the selector-based validation path.dcargvalues (including explicit null handling).Scope and type limitations
Tests
numbais available:test/python/CMakeLists.txtto add and activate apy:jitedtest group gated onnumba.test/python/jited.pyregistering a Numba-basedadder.addtransform over multiple NumPy dtypes and a verifier observer that asserts expected factors.test/python/pyjited.jsonnetto generate layers viadriver.cpp: 'generate_layers', usingcppsource4py, and mapping the module topy: 'jited'.test/python/adder.pyto assert an error whenaddtransform registration is missingoutput_product_suffixes(expectsTypeErrormentioning “should have an output suffix”).CI & security (CodeQL)
clang-tidy-fix.yaml,clang-format-fix.yaml,python-fix.yaml,cmake-format-fix.yaml,jsonnet-format-fix.yaml,coverage.yaml, and several*-check.yamljobs.